operations: add GPG key management operations (pr 1/3) #1460
Conversation
maisim
force-pushed
the
pr1-add-gpg-operations
branch
3 times, most recently
from
6521c5b to
5585d21
Compare
Add new gpg.key and gpg.dearmor operations to manage GPG keys and keyrings. These operations provide a modern alternative to apt-key for managing APT repository keys. Features: - Install keys from URLs, local files, or keyservers - Remove keys by ID or entire keyring files - Convert ASCII armored keys to binary format - Manage keys in specific keyrings or across all APT keyrings This is part 1/3 of modernizing APT key management.
maisim
force-pushed
the
pr1-add-gpg-operations
branch
from
5585d21 to
8d3ea39
Compare
|
Hi @DonDebonair ! |
|
Thanks for the PR @maisim . I would not be afraid to use facts inside operations. I actually consider it a best practice to use facts to check things in operations. So instead of doing checks inside the shell commands you yield, there are probably places where you can rely on facts instead. That makes the yielded commands simpler. If you can change those already, I'll do a more full review this weekend. Caveat btw: I'm by no means a GPG expert. A short while ago, I wanted to use pyinfra to install Docker on a Debian host. I just created the keyring directory and downloaded the key directly into that, never touching GPG 😅 |
| for kid in keyid: | ||
| # Remove key from all matching keyrings | ||
| yield ( | ||
| f'for keyring in {pattern}; do [ -e "$keyring" ] && ' |
There was a problem hiding this comment.
Doesn't it make more sense to retrieve keyrings through a fact and then only try to remove the key from keyrings that actually exist?
Or maybe even better, create a fact that lists all known keyrings and then use the GpgKeys fact for each of those keyrings to get all the keys. Then you can specifically remove the keyid if it exists
There was a problem hiding this comment.
Good idea. Since there’s no global or common location for keyrings, this will allow specifying the directories to search in — for example, /etc/apt/trusted.gpg.d/ and /usr/share/keyrings/ in the APT context.
|
|
||
| # Clean up empty keyrings | ||
| yield ( | ||
| f'for keyring in {pattern}; do [ -e "$keyring" ] && ' |
There was a problem hiding this comment.
As referenced in the other comment, if you can list all keys by keyring, then you know if keyrings are going to be empty after removing keyid from those rings and you can remove them based on that
| yield (f'if [ -f "{dest}" ] && ({condition}); then ' f'rm -f "{dest}"; fi') | ||
| return | ||
|
|
||
| elif dest and not keyid: |
There was a problem hiding this comment.
This could be else instead of elif. To me, that seems cleaner because it clearly signals there are only 3 possible combinations of conditions: keyid is provided and dest isn't, keyid and dest are both provided and lastly, dest is provided and keyid isn't
There was a problem hiding this comment.
Yes, but it is more an "documentation" elif. To be more explicit.
What about add an else wich raise an exception if we go into an unhandle case?
| elif dest and keyid: | ||
| # For APT keyring files, use a simpler approach: | ||
| # Check if keys exist in file, and if so, remove the entire file | ||
| # This is appropriate for most APT use cases |
There was a problem hiding this comment.
I'm a bit concerned about "most APT use cases" here. Is there a situation we might be removed a keyfile/keyring that has multiple keys in it?
|
|
||
|
|
||
| @operation() | ||
| def key( |
There was a problem hiding this comment.
This is a very complex function that covers many different scenarios. I think that from a public API standpoint, that is fine, because we're dealing with 1 resource: keys. But you could break up the function into separate functions that are all called from the key() function, based on the scenario.
See the docker operations for an example of how this can be done
|
Left some more comments @maisim incl. some ideas on how to leverate facts. |
2 participants
Add new gpg.key and gpg.dearmor operations to manage GPG keys and keyrings. These operations provide a modern alternative to apt-key for managing APT repository keys.
Features:
This is part 1/3 of modernizing APT key management.
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)